util: add default_value_vec for defaults without LengthReadable#4507
util: add default_value_vec for defaults without LengthReadable#4507carlaKC wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
I've assigned @jkczyz as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4507 +/- ##
==========================================
- Coverage 86.18% 86.17% -0.02%
==========================================
Files 160 160
Lines 107537 107562 +25
Branches 107537 107562 +25
==========================================
+ Hits 92681 92690 +9
- Misses 12229 12243 +14
- Partials 2627 2629 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/util/ser_macros.rs
Outdated
| items: Vec<u32>, | ||
| } | ||
| impl_writeable_tlv_based!(DefaultValueVecStruct, { | ||
| (1, items, (default_value_vec, Vec::new())), |
There was a problem hiding this comment.
nit: Might make sense to use a dummy default value here rather than the empty Vec to check this is actually what's used rather than Vec::default() in general?
Hmm, I guess we could use I'm not a huge fan of yet more write variants, ideally we'd drop some, really, but I do see why this is useful, and its not much of an extension on what we already have, so if |
Yeah we can, it's just pretty ugly, worth having the option IMO. |
Right now, use of `default_value` requires that the struct implements `LengthReadable` itself. When trying to use `default_value` outside of LDK for `Vec<T>`, your code will run into the orphan rule because it does not own the trait `LengthReadable` or the type `Vec`. There are various ugly workarounds for this (like using `custom`), but wanting to persist a vec with a default value seems like a common enough use case to justify the change.
carlaKC
force-pushed
the
default-vec-macro
branch
from
bcde6b5 to
1a24f8f
Compare
Oof, yea, that's not great. Happy to see this move forward. |
|
No issues found. I reviewed every macro arm in the diff across all 7 dispatch macros (
|
5 participants
In lightningdevkit/ldk-node#825, I ran into the orphan rule when trying to migrate from a single incoming/outgoing HTLC to a vec of
HTLCLocatortypes to allow expressing multiple in/out HTLCs. This issue stems from the following:HTLCLocatortype in LDK-Node, so that we can have niceties like a typeduser_channel_id.impl_writeable_tlv_based_enum!macro for LDK-Node'sEventtype.default_valueto fill the old fields when the new ones aren't present.However:
default_valueusesrequiredunder the hood, which expects the field to beLengthReadable.LengthReadableforVec<HTLCLocator>in LDK-Node due to the orphan rule (we don't ownVec<T>, even ifTis our type).impl_for_vec!to implement this.This change introduces a new
default_value_vecto our macro which usesrequired_vec(and thusWithoutLength) under the hood, so that we don't needLengthReadable. There are a few ugly workarounds (like usingcustom), but wanting to write aVecwith a default seems like a common enough one to justify a change to our macros.